Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'monitor_snapshot' cluster privilege #50489

Merged
merged 6 commits into from
Jan 6, 2020
Merged

Add 'monitor_snapshot' cluster privilege #50489

merged 6 commits into from
Jan 6, 2020

Conversation

AntonShuvaev
Copy link
Contributor

PR adds 'monitor_snapshot' cluster privilege.

Closes #50210

@dnhatn dnhatn added the :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC label Dec 24, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security (:Security/Authorization)

@dnhatn
Copy link
Member

dnhatn commented Dec 24, 2019

@elasticmachine ok to test

Copy link
Contributor

@albertzaharovits albertzaharovits left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you again for the contribution @J-Bean !

I see you've added a new cluster privilege monitor_snapshot as well as a new built-in role monitor_snapshot_user . But I don't think adding a built-in role is appropriate. We have too many of those and the addition of a new one, without a reasonable motivation, hurts the discoverability of the others. When we add a new built-in role we think of privileges that justifiably should go together, such as the privileges required to use a distinct Kibana feature (transform) or an Elastic Stack integration (Beats).

The case for the snapshot_user role, on which you've based your new role, is Curator. Curator requires that it can snapshot as well as list all the indices. In addition to supporting Curator, it would be incongruous for any other users to be able to snapshot indices that they cannot list. Given that at the moment we cannot limit the indices a user can snapshot, the next best alternative was to create a built-in role that grants snapshot and list permissions for all indices.

Given the above, I don't think there is a case for a new snapshot monitoring built-in role.

Other than that, the changes look good to me!

@tvernum tvernum self-requested a review January 2, 2020 06:43
Copy link
Contributor

@albertzaharovits albertzaharovits left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @J-Bean !
LGTM
We should wait for Tim to have his say before merging.

@tvernum
Copy link
Contributor

tvernum commented Jan 6, 2020

I think this change highlights a gap in our testing.
We don't have any tests that are able to check the difference between create_snapshot and monitor_snapshot because we only really run an integration test for that.

Separate to this PR, we need a non-integration ClusterPrivilege test that either goes privilege-by-privilege or action-by-action and verifies the correct access.

Copy link
Contributor

@tvernum tvernum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@albertzaharovits
Copy link
Contributor

@elasticmachine update branch

@albertzaharovits albertzaharovits merged commit 5da9175 into elastic:master Jan 6, 2020
albertzaharovits added a commit that referenced this pull request Jan 6, 2020
This adds a new cluster privilege `monitor_snapshot` which is a restricted
version of `create_snapshot`, granting the same privileges to view
snapshot and repository info and status but not granting the actual
privilege to create a snapshot.

Co-authored-by: j-bean <[email protected]>
@AntonShuvaev AntonShuvaev deleted the monitor_snapshot_privilege branch January 6, 2020 17:29
SivagurunathanV pushed a commit to SivagurunathanV/elasticsearch that referenced this pull request Jan 23, 2020
This adds a new cluster privilege `monitor_snapshot` which is a restricted
version of `create_snapshot`, granting the same privileges to view
snapshot and repository info and status but not granting the actual
privilege to create a snapshot. 

Co-authored-by: Anton Shuvaev <[email protected]>
@jakelandis jakelandis removed the v8.0.0 label Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add 'monitor_snapshot' cluster privilege
6 participants